488 include deviceregistration in devicedeploymentstatus
#489
Conversation
There was a problem hiding this comment.
Smaller comments I fixed in a fixup commit.
Some bigger observations:
- I would have expected a migration to be set up for
RecruitmentServicein the studies subsystem as well, which returnsParticipantGroupStatus, which containsStudyDeploymentStatusonce "invited". It's odd there are no failing tests on this. 🤔 - Extra test cases seem relevant (also see my comment on a bug which wasn't caught due to lacking coverage of
DeploymentService). But, if you add them for past versions (p.s. you added it to1.0and not1.1, was that intentional?), make sure they were generated with code for that version. You may have to branch out from an old release, write the test there, and then possibly do a cherry-pick on this branch.
....core/src/commonMain/kotlin/dk/cachet/carp/deployments/application/DeviceDeploymentStatus.kt
Outdated
Show resolved
Hide resolved
| "dk.cachet.carp.deployments.infrastructure.DeploymentServiceRequest.CreateStudyDeployment", | ||
| "dk.cachet.carp.deployments.infrastructure.DeploymentServiceRequest.GetStudyDeploymentStatus", | ||
| "dk.cachet.carp.deployments.infrastructure.DeploymentServiceRequest.UnregisterDevice", | ||
| "dk.cachet.carp.deployments.infrastructure.DeploymentServiceRequest.DeployDevice", |
There was a problem hiding this comment.
This should be DeviceDeployed. 🤔 Why does it work? I'm not a big TDD fan, but there is something to seeing a test fail before making it pass.
Seems like luck would have it that's exactly where coverage is missing. 😅 Could you add a preceding commit which provides coverage for DeploymentService.deviceDeployed()? And, while at it, verify that all the other endpoints of this service are actually called in the current regression tests.
There was a problem hiding this comment.
We only have coverage for CreateStudyDeployment, if I remember correctly that was the only failing test, as Actual testing of the status responses should already be covered adequately in StudyDeployment tests., but the regression test case are generated from DeploymentServiceTest.
Extra test cases seem relevant
Do we only want the generated json request files, or we also want those tests in DeploymentServiceTest? I think I'll include them first anyways to agree on the test cases, and they can be removed later if not desired.
I added it to 1.0 because I felt the test case was needed from the start of this major version, not due to some changes introduced in 1.1. Then simlly forget to add them 1.1 😛. As I generated those json files in ver. 1.0 and see a list of diff with origional ones, wandering what I breaked and realized those are the generated UUIDs difference...
There was a problem hiding this comment.
Do we only want the generated json request files, or we also want those tests in DeploymentServiceTest?
I'd always keep them in sync, so that it's just a matter of copying the output of all of them over.
But, obviously the comment I left in code there didn't consider the implications on reduced regression testing of the infrastructure layer (JSON) by not covering all potential input/output. 😅 So albeit valid—there is plenty of functional coverage in StudDeploymentTest—, that doesn't cover serialization and JSON schema definitions.
I think it would make sense to write test cases that try to cover as much as possible, rather than writing them as unit tests. These are more or less integration tests after all. So, I'd focus on longer scenarios with multiple calls reflecting fuller study lifetime to get more coverage for the infrastructure layer.
As I generated those json files in ver. 1.0 and see a list of diff with origional ones, wandering what I breaked and realized those are the generated UUIDs difference...
I actually spotted that. 🤔. I thought I injected some more deterministic GUID and timestamp generator for the creation of these tests. I saw it in some test resources, but not others. I don't remember whether that's expected/normal, or oversight.
.../kotlin/dk/cachet/carp/deployments/infrastructure/versioning/DeploymentServiceApiMigrator.kt
Outdated
Show resolved
Hide resolved
.../kotlin/dk/cachet/carp/deployments/infrastructure/versioning/DeploymentServiceApiMigrator.kt
Outdated
Show resolved
Hide resolved
DeviceRegistrationis added toDeviceDeploymentStatus, which is part ofStudyDeploymentStatus. A new interfaceHasDeviceRegistrationis introduced to keep this field sinceUnregisteredwould not need this field.A regression test case was added in a separate commit as I'm not sure if it is desired. The test case amis to test
GetStudyDeploymentStatusListendpoint with registered devices in one of the deployments. This is not covered sinceDeploymentServiceTesttests minimum usage forGetStudyDeploymentStatusList, as it is just a list representation ofGetDeploymentStatus. I added it since json migration for this endpoint works differently.